Dispose exec children once on every TUI exit path - #846
Conversation
TheGreatAxios
left a comment
There was a problem hiding this comment.
independent critic found no blocking orphans on primary TUI/exec run_shell paths. Once-only dispose holds. Follow-ups already in: quit shuts down before hung tail, reap timeout fails teardown, shutdown errors logged. Remaining should-fix being addressed: subagent dispose currently swallows posixTools.dispose rejection.
TheGreatAxios
left a comment
There was a problem hiding this comment.
re-critic of origin/main..HEAD including ed15287 found no blocking orphans on primary TUI/exec run_shell paths. Subagent posix dispose rejection is no longer swallowed on ephemeral teardown (disposeSubAgentSession awaits with no catch). Remaining file-for-later: persist close_agent still best-effort-catches dispose, so a persist worker leftover after reap can still look like success at the fleet layer.
TheGreatAxios
left a comment
There was a problem hiding this comment.
Critic · Request changes
TUI/exec teardown is once-only and leftover posix dispose can fail the close path, but live children are still SIGKILLed only after agent.close(), so a wedged close or the 2s crash/signal race orphans detached shell-guard processes.
Findings
-
src/subagent/dispose.ts:113/src/subagent/run.ts:1074—posixTools.dispose()(the process-group kill) runs only afterawait agent.close()andawait streamPromise.closeOnAbortno longer disposes posix tools.boundedClosethen times out as success (src/subagent/run.ts:1111,src/subagent/session-store.ts:30). Sequence: persist worker with a liverun_shellchild;agent.close()hangs (the documented close-ordering wedge);close_agent/cancelAllhits the deadline; session isshutdown; the detached child is still alive.src/subagent/session-store.test.ts:572locks in hung-close → shutdown success. -
src/tui/runner/shutdown.ts:44/src/exec/runner.ts:171/src/index.ts:123— crash/signalPromise.racewaits 2s for the whole chain (cancelAll→closeAgent→disposeToolset). Reap is the last leg. Sequence: live parentrun_shell; SIGTERM whileagent.close()takes >2s;awaitActiveDisposeHostlogs the deadline andprocess.exits; detached children survive. The integration fixture usesagent: null, so it never hits this. -
src/index.ts:129— the 2s timer is never cleared when dispose wins. The loser rejects later asunhandledRejection(handler no-ops becauseterminatingis already true). -
src/exec/runner.ts:891— leftover dispose after a successful exec islogger.debugplusexitCode = 1. No stderr. Operator sees a silent 1;run.jsonis alreadydone. -
src/tui/runner/exit.ts:544— shutdown now precedesawaitTail, but the tail is still unbounded. A hung session-op still preventsrun.json, telemetry, and the teardownFailed exit 1.src/tui/runner/exit.test.ts:56documents the hang. -
src/tui/runner/shutdown.ts:18—completionstarts asPromise.resolve(). A signal that re-enters afterstarted = trueand beforecompletion = (async () => …)()(window isdisposeHost()+ startingcancelWorkers) observes a finished dummy and canprocess.exitbefore reap.
Notes
- Once-only plugin dispose, leftover throw from
reapLiveChildren, and TUIteardownFailed→ exit 1 are fine when close actually returns in time. - Permanent tests to keep: hung
agent.close()with a live tracked child must kill or fail close, not report shutdown; signal path with slowcloseAgentmust still reap; successful exec + throwing toolset dispose must be exit 1 and visible.
|
re-critic of origin/main..HEAD at 1f6c66c found no remaining blocking leftover-as-success swallow on TUI quit, disposeExecRuntime, persist close_agent, or parent toolset dispose. cancelAll now awaits retained close; leftover posix dispose rejects. Remaining should-fix, not blocking: exec runExec maps that reject to exitCode 1 with status still "done" and a debug-only log (no stderr); createAgentToolset.dispose stops the fleet walk after the first leftover and skips parent posixTools.dispose. |
|
re-critic of origin/main..HEAD at 7a2ab30 found both remaining should-fixes closed. Leftover exec dispose now fails the ExecResult (exit 1, status failed, error + stderr). Parent toolset dispose finishes remaining closeOne and posix teardown before rethrowing. No blocking leftover-as-success swallow remains on TUI quit, disposeExecRuntime, persist close_agent, or runExec. File-for-later only: on-disk persist("done") / TUI run.json still record a completed send when teardown later fails. |
694e5af to
c2a26d3
Compare
Stop workers first so a hung session-op cannot delay abort and reap. Log shutdown failures at error while still mapping teardown failure to exit 1.
A leftover after the two-second backstop must reject dispose so the exit 1 path can fire. Abort already SIGKILLs the process group at abort start.
A leftover child after posix reap must fail persist close and parent toolset dispose instead of looking like a successful shutdown. A wedged close still times out as shutdown.
A hung agent.close used to run before process-group reap, so teardown could report success while detached run_shell children were still live. Dispose first, fail a close deadline instead of succeeding, and clear the two-second host timer when dispose wins.
Latch dispose so overlapping calls join one reap, and refuse queued shells that would spawn after the guard is gone.
c2a26d3 to
9a74540
Compare
Summary
Verification
bun run typecheck,bun run build, andbun run testpassbun run checkpassesFixes CL-7534
Changelog: bug